Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add setSessionId() method #95

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

falconandy
Copy link
Contributor

Summary

Adds setSessionId() method.

Typical usage is amplitude.setSessionId(timestamp: Int64(NSDate().timeIntervalSince1970 * 1000))

Maybe it makes sense to add:

  • method startNewSession() (without any parameters, the same as setSessionId(timestamp: Int64(NSDate().timeIntervalSince1970 * 1000)))
  • method endCurrentSession() to end the current session but do not start new session. Can be used in client Log Out logic. Maybe it can be used inside SDK reset() method.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: No

@justin-fiedler
Copy link
Contributor

justin-fiedler commented Oct 25, 2023

Typical usage is amplitude.setSessionId(timestamp: Int64(NSDate().timeIntervalSince1970 * 1000))

Maybe it makes sense to add:

  • method startNewSession() (without any parameters, the same as setSessionId(timestamp: Int64(NSDate().timeIntervalSince1970 * 1000)))
  • method endCurrentSession() to end the current session but do not start new session. Can be used in client Log Out logic. Maybe it can be used inside SDK reset() method.

Thanks @falconandy. I agree the current usage is pretty clunky.

A couple options:

  1. New methods like you suggest. I would prefer naming them sessionStart(timestamp? = Date.now()) and sessionEnd() to keep them grouped in IDE autocomplete. wdyt?
  2. New convenience method setSessionId(date: Date)) with usage amplitude.setSessionId(date: Date())

I like that #2 as it's a smaller footprint, but I think #1 is easier to understand.

I am leaning towards #1, or both. Wdyt?

@falconandy
Copy link
Contributor Author

@justin-fiedler
I added all 3 methods.

Parameter naming (wdyt?):
For setSessionId() the parameter is timestamp: Int64
For sessionStart() the parameter is optional at: Date = Date()
For sessionEnd() the parameter is optional at: Date? = nil. nil means last known event time but explicit value can be set to use current time, for example. Also explicit at value is used in tests.

@justin-fiedler
Copy link
Contributor

justin-fiedler commented Oct 26, 2023

I added all 3 methods.

Parameter naming (wdyt?): For setSessionId() the parameter is timestamp: Int64 For sessionStart() the parameter is optional at: Date = Date() For sessionEnd() the parameter is optional at: Date? = nil. nil means last known event time but explicit value can be set to use current time, for example. Also explicit at value is used in tests.

Thanks @falconandy!

I thought about this more and discussed with the team. We feel that option 2 is the way to go for these reasons

  1. Setting the session id is viewed as "advanced" usage. In general sessions are automatically handled by the SDK and we don't want customers to feel they need to do this themselves. By adding sessionStart() and sessionEnd() it makes this functionality more visible to customers and they then may try to manange sessions themselves which could cause more harm than good.
  2. Less methods to support/document
  3. Keeps the same interface for all SDKs

TLDR; Can you please

  1. Remove sessionStart() and sessionEnd()
  2. Add setSessionId(date: Date) method. Note no default value for date.
  3. Keep setSessionId(timestamp: Int64).

Thank you!

@falconandy
Copy link
Contributor Author

@justin-fiedler
setSessionId() method always starts new session. Are there any cases when current session should be ended and new session shouldn't be started? For example, on logout event. New session would be started later, on login event.

@justin-fiedler
Copy link
Contributor

justin-fiedler commented Oct 26, 2023

setSessionId() method always starts new session. Are there any cases when current session should be ended and new session shouldn't be started? For example, on logout event. New session would be started later, on login event.

Thanks @falconandy for this case I believe customers can use setSessionId(timestamp: -1) and later setSessionId(date: Date())

@falconandy
Copy link
Contributor Author

@justin-fiedler
ok, sessionStart/sessionEnd are removed, setSessionId(timestamp: - 1) support is added
Next question: how long the explicit session Id (-1 as a special case) should be preserved and how/when return to auto-tracking?

  1. Forever.
  2. Until app restart.
  3. Until app background/foreground
  4. Until next event (for session -1).
  5. etc

Right now usual session logic works:

  • setSession(-1) ends current session, any next event starts new session.
  • setSession(timestamp) ends current session, starts new session, session logic works as before (minTimeBetweenSessionMillis can start new session)

@justin-fiedler justin-fiedler merged commit 1fb9f01 into main Oct 27, 2023
3 checks passed
@justin-fiedler justin-fiedler deleted the AMP-87537-add-setSessionId-method branch October 27, 2023 16:18
github-actions bot pushed a commit that referenced this pull request Oct 27, 2023
# [1.0.0](v0.7.3...v1.0.0) (2023-10-27)

### Bug Fixes

* add setSessionId() method ([#95](#95)) ([1fb9f01](1fb9f01))

### Features

* Release 1.0.0 ([#98](#98)) ([b047273](b047273))

### BREAKING CHANGES

* New major version
@github-actions
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants